gh-157468: Validate correct builtins used under JIT - #157766
Conversation
f72d497 to
24c909d
Compare
- Test 1: verifying the traceguard is correctly activating - Test 2: verifying the runtime guard is activating
24c909d to
f77395f
Compare
cocolato
left a comment
There was a problem hiding this comment.
Please don't force push PRs. We‘ll squash merge commits at the end. And some comments:
cocolato
left a comment
There was a problem hiding this comment.
Thanks for doing this, there are also a few comments
Co-authored-by: Hai Zhu <haiizhu@outlook.com>
cocolato
left a comment
There was a problem hiding this comment.
LGTM, and let's wait a core dev to review this
| if (ctx->frame->globals_checked_version != 0 && ctx->frame->globals_watched) { | ||
| if (ctx->frame->globals_checked_version != 0 && | ||
| ctx->frame->globals_watched && | ||
| uop_buffer_remaining_space(&ctx->out_buffer) >= 2) |
There was a problem hiding this comment.
We always allow enough headroom for small changes like this. No need to check here.
|
I reverted the last commit against my fork and the same test failed so don't think it's related (and didn't seem to be anyway) 1 test failed: Edit: Saying that main appears to be in a good state although confused about the failure here |
|
The failing test is now passing after latest merge to main @cocolato @markshannon I did some analysis and looks to be related to the code layout generated and whether a |
|
Isn't |
Co-authored-by: Hai Zhu <haiizhu@outlook.com>
@johng refer: #157468 (comment) |
markshannon
left a comment
There was a problem hiding this comment.
One small nit, otherwise looks good
| /* Do nothing */ | ||
| } | ||
| else if (ctx->frame->func == NULL || | ||
| ctx->frame->func->func_builtins != builtins) { |
There was a problem hiding this comment.
Empty clause. Can you put a /* Do nothing */ comment here for clarity
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
Thanks @johng for the PR, and @markshannon for merging it 🌮🎉.. I'm working now to backport this PR to: 3.15. |
|
Sorry, @johng and @markshannon, I could not cleanly backport this to |
|
Thanks for the reviews! |
|
|
This adds the two checks at tracing and at JIT runtime that the builtin dict is the original interpreter's builtins